pkg/k8s/alert_relabel_config.go (1)

95-102: Inconsistent error message in Delete - missing namespace.

The Create and Update methods include both namespace and name in their error messages (%s/%s), but Delete only includes the name. This inconsistency could make debugging harder when multiple namespaces contain resources with the same name.

Proposed fix

 func (arcm *alertRelabelConfigManager) Delete(ctx context.Context, namespace string, name string) error {
 	err := arcm.clientset.MonitoringV1().AlertRelabelConfigs(namespace).Delete(ctx, name, metav1.DeleteOptions{})
 	if err != nil {
-		return fmt.Errorf("failed to delete AlertRelabelConfig %s: %w", name, err)
+		return fmt.Errorf("failed to delete AlertRelabelConfig %s/%s: %w", namespace, name, err)
 	}
 
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/k8s/alert_relabel_config.go` around lines 95 - 102, The Delete method on
alertRelabelConfigManager returns an error message that only includes the
resource name; update alertRelabelConfigManager.Delete to include both namespace
and name in the error string (matching the Create/Update pattern) by formatting
the error as "%s/%s" (namespace/name) when wrapping the returned error from
arcm.clientset.MonitoringV1().AlertRelabelConfigs(namespace).Delete; ensure the
error message text mirrors the existing style used in Create/Update for
consistency.

internal/managementrouter/create_alert_rule.go (1)

21-26: Consider limiting request body size.

The handler decodes the request body without size limits. A malicious client could send an extremely large payload, potentially causing memory exhaustion. Consider using http.MaxBytesReader to limit the request body size.

Proposed fix

 func (hr *httpRouter) CreateAlertRule(w http.ResponseWriter, req *http.Request) {
+	// Limit request body to 1MB to prevent memory exhaustion
+	req.Body = http.MaxBytesReader(w, req.Body, 1<<20)
+
 	var payload CreateAlertRuleRequest
 	if err := json.NewDecoder(req.Body).Decode(&payload); err != nil {
 		writeError(w, http.StatusBadRequest, "invalid request body")
 		return
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/managementrouter/create_alert_rule.go` around lines 21 - 26, The
CreateAlertRule handler decodes req.Body without any size limit, risking memory
exhaustion; wrap the request body with http.MaxBytesReader (e.g., maxBytes
constant like 1<<20 for 1MB) before decoding in CreateAlertRule on the
httpRouter, then decode into CreateAlertRuleRequest from that wrapped reader and
handle a size-exceeded error by calling writeError(w,
http.StatusRequestEntityTooLarge, "...") (or appropriate message) so overly
large payloads are rejected gracefully.

internal/managementrouter/router.go (1)

62-64: Consider limiting error details in 500 responses.

The current implementation returns the full internal error message to the client on unexpected errors. While this aids debugging, it may leak implementation details (e.g., database connection strings, internal paths) in production.

Proposed fix - return generic message to client

 	log.Printf("An unexpected error occurred: %v", err)
-	return http.StatusInternalServerError, fmt.Sprintf("An unexpected error occurred: %s", err.Error())
+	return http.StatusInternalServerError, "An unexpected error occurred"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/managementrouter/router.go` around lines 62 - 64, The handler
currently returns the full internal error string in 500 responses; update the
code that returns http.StatusInternalServerError (the block using log.Printf("An
unexpected error occurred: %v", err) and return http.StatusInternalServerError,
fmt.Sprintf("An unexpected error occurred: %s", err.Error())) to keep detailed
error logging server-side but send a generic client message instead (e.g., log
the full err with log.Printf or a logger, then return
http.StatusInternalServerError and a constant message like "An unexpected error
occurred" without including err.Error()) so internal details aren’t leaked.

pkg/alert_rule/alert_rule.go (1)

69-71: Minor: redundant TrimSpace call.

key was already trimmed on line 61. The second TrimSpace on line 69 is unnecessary.

♻️ Suggested simplification

-		if !promLabelNameRegexp.MatchString(strings.TrimSpace(key)) {
+		if !promLabelNameRegexp.MatchString(key) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/alert_rule/alert_rule.go` around lines 69 - 71, The second
strings.TrimSpace call is redundant because key was already trimmed earlier; in
the block using promLabelNameRegexp.MatchString replace strings.TrimSpace(key)
with key to avoid unnecessary allocation while preserving behavior—look for the
check involving promLabelNameRegexp and the variable key in alert_rule.go and
remove the extra TrimSpace.

pkg/management/create_platform_alert_rule.go (1)

101-119: Consider documenting the alert name conflict check behavior.

addRuleToGroup checks for alert name conflicts (line 107) but not for spec equivalence (unlike CreateUserDefinedAlertRule which checks both). This means two platform rules with different names but identical specs could potentially be added.

The duplicate check on line 29-31 via RelabeledRules().Get() should catch exact ID matches, but the relabeled rules cache is eventually consistent. If this asymmetry is intentional, a brief comment explaining the design choice would help future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/management/create_platform_alert_rule.go` around lines 101 - 119, The
addRuleToGroup function only prevents duplicate alert names (in addRuleToGroup)
but not duplicate rule specs (unlike CreateUserDefinedAlertRule which checks
spec equivalence), creating an intentional asymmetry due to eventual consistency
of RelabeledRules().Get(); update the code by adding a concise in-code comment
inside addRuleToGroup (near the name-conflict check and before appending the new
group) that explains this design choice: that we only check Alert name here,
that spec equivalence is validated elsewhere (CreateUserDefinedAlertRule and via
RelabeledRules().Get()), and that the relabeled cache is eventually consistent
so duplicate-spec prevention is handled at a different layer rather than here;
reference addRuleToGroup and RelabeledRules().Get() in the comment so future
maintainers can locate the related logic.

pkg/management/create_user_defined_alert_rule.go (1)

97-124: Consider normalizing expression comparison for equivalence check.

rulesHaveEquivalentSpec compares expressions via a.Expr.String() != b.Expr.String(), which is sensitive to whitespace differences. However, GetAlertingRuleId normalizes expressions (collapses whitespace) before hashing.

This means two rules with equivalent normalized expressions but different whitespace would:

• Have the same computed ID (duplicate detected at line 32-35)
• But not be flagged as equivalent spec here (lines 100-102)

If this is intentional (exact byte-for-byte match for spec equivalence), it's fine. Otherwise, consider reusing normalizeExpr:

+import alertrule "github.com/openshift/monitoring-plugin/pkg/alert_rule"
+
 func rulesHaveEquivalentSpec(a, b monitoringv1.Rule) bool {
-	if a.Expr.String() != b.Expr.String() {
+	if alertrule.NormalizeExpr(a.Expr.String()) != alertrule.NormalizeExpr(b.Expr.String()) {
 		return false
 	}

Note: This would require exporting normalizeExpr from alertrule package.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/management/create_user_defined_alert_rule.go` around lines 97 - 124,
rulesHaveEquivalentSpec currently compares PromQL expressions with a raw string
comparison (a.Expr.String() != b.Expr.String()), which is whitespace-sensitive
and can mismatch rules that GetAlertingRuleId treats as identical; change
rulesHaveEquivalentSpec to compare normalized expressions by applying the same
normalizeExpr used by GetAlertingRuleId (or make normalizeExpr exported from the
alertrule package and call alertrule.NormalizeExpr here), then compare the
normalized strings; ensure imports are updated and keep existing checks for For
and filterBusinessLabels unchanged so equivalence now uses normalized
expressions.

pkg/k8s/alerting_rule.go (1)

100-107: Consider idempotent delete behavior for NotFound errors.

The Delete method returns an error when the resource is not found, whereas Get returns (nil, false, nil) for the same case. For idempotent delete semantics (common in reconciliation loops), you might want to suppress NotFound errors:

 func (arm *alertingRuleManager) Delete(ctx context.Context, name string) error {
 	err := arm.clientset.MonitoringV1().AlertingRules(ClusterMonitoringNamespace).Delete(ctx, name, metav1.DeleteOptions{})
 	if err != nil {
+		if errors.IsNotFound(err) {
+			return nil
+		}
 		return fmt.Errorf("failed to delete AlertingRule %s/%s: %w", ClusterMonitoringNamespace, name, err)
 	}
 
 	return nil
 }

If callers need to know whether the resource existed, this change would be inappropriate—keep the current behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/k8s/alerting_rule.go` around lines 100 - 107, The Delete method on
alertingRuleManager returns an error when the resource is NotFound; change it to
be idempotent by checking the delete error from
arm.clientset.MonitoringV1().AlertingRules(ClusterMonitoringNamespace).Delete(...)
and return nil if it is a NotFound error (use apierrors.IsNotFound). Ensure you
add the apierrors import (k8s.io/apimachinery/pkg/api/errors) and keep returning
wrapped errors for other failure cases so only NotFound is suppressed.

pkg/management/create_platform_alert_rule_test.go (1)

97-123: Consider adding test for operator-managed AlertingRule rejection.

The test suite covers GitOps-managed rejection (lines 98-123), but there's no test case for operator-managed AlertingRule rejection. The implementation in create_platform_alert_rule.go (lines 47-51) handles both cases, but only one is tested.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/management/create_platform_alert_rule_test.go` around lines 97 - 123, Add
a test case parallel to the GitOps-managed one that verifies
CreatePlatformAlertRule rejects operator-managed AlertingRule: in
pkg/management/create_platform_alert_rule_test.go add an It block under the
existing Context("when target AlertingRule exists") that configures
mockK8s.AlertingRulesFunc to return an osmv1.AlertingRule whose ObjectMeta
contains the operator-management annotation/key that
create_platform_alert_rule.go checks (the same annotation/key used in the
implementation), call client.CreatePlatformAlertRule(ctx, baseRule), and assert
the call returns an error containing the same "managed by" rejection message;
mirror the structure of the existing GitOps test and use the same
MockAlertingRuleInterface GetFunc pattern so the operator-managed branch in
CreatePlatformAlertRule is exercised.